Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #388 Triggering Authenticator double encodes body in POST, PUT, PATCH #390

Merged
merged 6 commits into from
Dec 11, 2022
Merged

Fix #388 Triggering Authenticator double encodes body in POST, PUT, PATCH #390

merged 6 commits into from
Dec 11, 2022

Conversation

techouse
Copy link
Collaborator

@techouse techouse commented Dec 10, 2022 •
edited
Loading

This fixes #388

When ChopperClient is configured with an Authenticator and when Authenticator.authenticate returns a non-null value, ChopperClient.send recursively passes an already converted Request to ChopperClient.send again. This results in double-encoded JSON.

Expected: '{"name":"john","surname":"doe"}'
  Actual: '"{\\"name\\":\\"john\\",\\"surname\\":\\"doe\\"}"'
   Which: is different.
          Expected: {"name":"j ...
            Actual: "{\\"name\ ...
                    ^
           Differ at offset 0

As previously mentioned, the issue only manifests itself whenAuthenticator.authenticate returns a non-null value, in essence when the client receives an HTTP ERROR: 401. It works perfectly fine when the client is authenticated and/or the Auth token hasn't expired. That's why it probably flew under the radar for so long.

This PR fixes it by passing the original Request to ChopperClient.send in that case.

@techouse techouse mentioned this pull request Dec 10, 2022
Closed
JEuler
JEuler approved these changes Dec 10, 2022
View reviewed changes
Copy link
Collaborator

@JEuler JEuler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's so cool you've added tests! <3 Thank you!

@techouse
Copy link
Collaborator Author

That's so cool you've added tests! <3 Thank you!

Yeah, I was only able to replicate the issues with tests. After confirming the issue with tests I proceeded to fix it. TDD I guess 🙈

@techouse techouse added the bug Something isn't working label Dec 11, 2022
@JEuler
Copy link
Collaborator

JEuler commented Dec 11, 2022

Should I merge it or you will? :)

@techouse
Copy link
Collaborator Author

Should I merge it or you will? :)

You're the merge master 😎

@JEuler JEuler merged commit 11a9ca5 into lejard-h:develop Dec 11, 2022
@techouse techouse deleted the fix-authenticator-corrupting-body branch December 11, 2022 12:14
@techouse
Copy link
Collaborator Author

@JEuler when are we getting this fix to origin/master?

@JEuler
Copy link
Collaborator

JEuler commented Dec 12, 2022

Hm, let me prepare PR tomorrow. :)

@JEuler
Copy link
Collaborator

JEuler commented Dec 13, 2022

What do you think about the version?
b0fd18f - I think we need to bump major version as I understand and remember.

@techouse
Copy link
Collaborator Author

Probably v5.2.0

@JEuler
Copy link
Collaborator

JEuler commented Dec 13, 2022

Okay, I will do it today. :)

@techouse techouse mentioned this pull request Dec 14, 2022
Closed
@techouse techouse mentioned this pull request Jul 8, 2023
Closed
This was referenced Aug 26, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JEuler JEuler JEuler approved these changes

@lejard-h lejard-h Awaiting requested review from lejard-h lejard-h is a code owner

@stewemetal stewemetal Awaiting requested review from stewemetal stewemetal is a code owner

Assignees

@JEuler JEuler

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Authenticator corrupts request body
2 participants
@techouse @JEuler